feat: chacha20 #51
feat: chacha20 #51
Conversation
0xJepsen
requested review from
lonerapier and
Autoparallel
and removed request for
lonerapier
|
lol the name here |
This reverts commit 8aa8b08.
Co-authored-by: Colin Roberts <[email protected]>
0xJepsen
force-pushed
the
revert-50-revert-49-chacha20
branch
from
f0dc7e4 to
ea6ab02
Compare
|
|
||
| // the below can be both ciphertext or plaintext depending on the direction | ||
| // in => N 32-bit words => N 4 byte words | ||
| signal input plainText[N][32]; |
There was a problem hiding this comment.
I think it would be preferable to have this take in bytes instead of bits. But maybe its not a big deal at all.
There was a problem hiding this comment.
yes i understand. Doing so is not as straight forward as we would like as the 32 bit words are little endian encoded. I did some work to start on this but will make an issue.
There was a problem hiding this comment.
Okay, in my opinion this can be merged so we can move along, but I honestly feel like this could be made quite a bit cleaner and clearer.
For one, I think it would probably serve us to have ChaCha20 take in bytes, not bits. Would be as easy as calling ByteToBits or ToBits(8) or whatever the template is. This makes life easier on the Rust side. It's not a big deal either way I suppose, but this just changes the API in an unexpected way, in my opinion.
Second, the way the full tests were laid out is, bizarre. If I was being stricter here, I'd ask you to redo it in the following way:
- Leave the
NIVC_FULLandNIVC_FULL_2tests as is, but rename them toNIVC_FULL_AESandNIVC_FULL_2_AES. - Create two tests
NIVC_FULL_CHACHAandNIVC_FULL_2_CHACHA. - I think we should be testing these circuits in the same way we tested AES-GCTR here. The first
NIVC_FULLfolds a single 16 byte chunk at a time,NIVC_FULLfolds 32 bytes at a time. You could mimic this with ChaCha by havingNIVC_FULL_CHACHArun with all 320 bytes at once. ThenNIVC_FULL_CHACHA_2could do 160 bytes at once. This tests that the parameter for how many 32bit words to fold at once works with our circuit chain. It almost certainly does, but why not just test it that way?
Please consider taking this feedback and integrating it before you merge this. If not, then make issues at least, please!
Reverts #50
Closes #54
This one is reopen so now you can review